Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix 547 0.27 #1210

Merged
merged 17 commits into from
May 20, 2020
Merged

Fix 547 0.27 #1210

merged 17 commits into from
May 20, 2020

Conversation

clanmills
Copy link
Collaborator

@clanmills clanmills commented May 19, 2020 •
edited
Loading

Fix: #547

The -pR option is only provided in debug builds to assist users in debugging files and cannot be called on production builds.

This fix uses a thread unsafe static and should not be ported to v0.28. We could
Either: modify the API of printIFDStructure() to include std::set<long>& visits
Or: have the std::set<long> visits in the Image class.

I cannot use either choice in v0.27.3 because printIFDStructure() and the Image class are exposed in the Exiv2 dynamic library. Changing will break DLL swap/compatibility.

...build $ nm -g --demangle lib/libexiv2.27.dylib | grep printIFDStructure
00000000000825f0 T Exiv2::Image::printIFDStructure(Exiv2::BasicIo&, std::__1::basic_ostream<char, std::__1::char_traits<char> >&, Exiv2::PrintStructureOption, unsigned int, bool, char, int)
...build $

@clanmills clanmills added this to the v0.27.3 milestone May 19, 2020
@clanmills clanmills self-assigned this May 19, 2020
@clanmills clanmills mentioned this pull request May 19, 2020
Closed
@clanmills clanmills mentioned this pull request May 19, 2020
Open
piponazo
piponazo previously approved these changes May 19, 2020
View reviewed changes
@mergify mergify bot dismissed piponazo’s stale review May 19, 2020 20:57

Pull request has been modified.

@codecov
Copy link

codecov bot commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #1210 into 0.27-maintenance will increase coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@                Coverage Diff                @@
##           0.27-maintenance    #1210   +/-   ##
=================================================
  Coverage             63.47%   63.48%           
=================================================
  Files                   159      159           
  Lines                 22877    22882    +5     
=================================================
+ Hits                  14522    14526    +4     
- Misses                 8355     8356    +1
Impacted Files Coverage Δ
src/image.cpp 78.50% <80.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6ccb8e...1b82c6a. Read the comment docs.

@piponazo
Copy link
Collaborator

Hi Robin. It seems the modifications you are doing in the file ci/test_build.py are affecting to the compilation of gtest from its sources in OpenSUSE. This reminds me to the issue reported few weeks about about gtest & C++98 vs C++11 (#1163). Aparently OpenSUSE is using the latest version of gtest that requires C++11 and that is why you are getting errors like this one:

 /usr/include/gtest/gtest.h:2445:53: note: ‘std::move’ is only available from C++11 onwards

https://gitlab.com/D4N/exiv2/-/jobs/559811212

@clanmills
Copy link
Collaborator Author

clanmills commented May 20, 2020 •
edited
Loading

That's exactly what's wrong. I'm trying to understand the man page for zypper. If we build with C++98, we must use GTest 1.8 (as I have documented in #575).

@clanmills
Copy link
Collaborator Author

I'm installing an OpenSUSE VM. I need to run the command zypper search -s gtest to discover exactly what zypper needs!

@clanmills
Copy link
Collaborator Author

It should be zypper install gtest=1.8.0-lp151.2.3

@clanmills
Copy link
Collaborator Author

I installed openSUSE-Leap and he offered gtest=1.8.0-lp151.2.3

I've installed openSUSE-Tumbleweed. He's only offering 1.10

1431 rmills@rmillsmbp:~/gnu/github/exiv2/0.27-maintenance $ ssh rmillsmm 'cat ~/temp/foo.txt'
S | Name  | Type    | Version    | Arch   | Repository
--+-------+---------+------------+--------+------------------------
  | gtest | package | 1.10.0-1.1 | x86_64 | openSUSE-Tumbleweed-Oss
  | gtest | package | 1.10.0-1.1 | i586   | openSUSE-Tumbleweed-Oss
rmills@rmillsmm-opensuset:~>

Looks like the curl/tar dance is the way to go for CPP/98 on v0.27. We shouldn't port the CI changes to v0.28.

We should not port the changes to image.cpp to v0.28. I'll open a new issue about this for v0.28.

@clanmills clanmills mentioned this pull request May 20, 2020
Closed
@clanmills clanmills merged commit f16c4c6 into 0.27-maintenance May 20, 2020
@clanmills
Copy link
Collaborator Author

I'm not going to wait for AppVeyor to build on Windows. I've tested that on the Mac-mini.

@clanmills clanmills deleted the fix_547_0.27 branch May 20, 2020 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piponazo piponazo piponazo left review comments

Assignees

@clanmills clanmills

Labels
None yet
Projects
None yet
Milestone
v0.27.3
Development

Successfully merging this pull request may close these issues.
2 participants
@clanmills @piponazo